Dates: Accept ISO-8601 format, and use UTC milliseconds as the backend#1194
Merged
Conversation
We still interpret JS date objects according to the date and hour they have in the local timezone, and for backward compatibility we shift milliseconds (in ranges etc) by the local/UTC offset.
alexcjohnson
added
status: reviewable
bug
etpinard
reviewed
Nov 24, 2016
| var ONEMIN = constants.ONEMIN; | ||
| var ONESEC = constants.ONESEC; | ||
|
|
||
| var DATETIME_REGEXP = /^\s*(-?\d\d\d\d|\d\d)(-(0?[1-9]|1[012])(-([0-3]?\d)([ Tt]([01]?\d|2[0-3])(:([0-5]\d)(:([0-5]\d(\.\d+)?))?(Z|z|[+\-]\d\d:?\d\d)?)?)?)?)?\s*$/m; |
Contributor
There was a problem hiding this comment.
Did you cook this up yourself or find from a google search? If the latter, can you paste the reference above?
| '2015-01-01 12:00:60', '2015-01-01 12:00:-1', '2015-01-01 12:00:001', '2015-01-01 12:00:1' // bad second | ||
| '2015-01-01 12:00:60', '2015-01-01 12:00:-1', '2015-01-01 12:00:001', '2015-01-01 12:00:1', // bad second | ||
| '2015-01-01T', '2015-01-01TT12:34', // bad ISO separators | ||
| '2015-01-01Z', '2015-01-01T12Z', '2015-01-01T12:34Z05:00', '2015-01-01 12:34+500', '2015-01-01 12:34-5:00' // bad TZ info |
etpinard
reviewed
Nov 24, 2016
| new Date(), | ||
| new Date(-9999, 0, 1), | ||
| new Date(9999, 11, 31, 23, 59, 59, 999) | ||
| new Date(-9999, 0, 3), // we lose one day of range +/- tzoffset this way |
etpinard
reviewed
Nov 24, 2016
| xaxis: { | ||
| type: 'date', | ||
| range: ['2000-01-01', (new Date(2000, 1, 2)).getTime()] | ||
| range: ['2000-01-01', '2000-02-02'] |
etpinard
reviewed
Nov 24, 2016
etpinard
left a comment
etpinard
left a comment
Contributor
There was a problem hiding this comment.
Looking good 💃
I'm thinking it would be nice to make http://codepen.io/plotly/pen/grdEaN (from #811) part of our image tests. Non-blocking if you feel that the jasmine tests are sufficient.
Contributor
alexcjohnson
commented
Nov 25, 2016
| "xaxis4": {"anchor": "y4"}, | ||
| "xaxis5": {"anchor": "y5"}, | ||
| "xaxis6": {"anchor": "y6"}, | ||
| "xaxis7": {"anchor": "y7"} |
Collaborator
Author
There was a problem hiding this comment.
@etpinard good point about including the UTC behavior in the mocks. I replaced the simple date_axes.json with this one that includes tick scales all the way from full years to partial seconds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First part: ISO-8601:
fixes #1003
For simplicity, I ended up separately allowing the date/time separator to be space, t, or T, and a timezone spec at the end - ie timezones are now accepted for our normal space-separated format too.
Also @etpinard a perf note - I switched from the manual string chopping we had before to a regexp-based conversion. When I originally wrote this converter a few years ago it was clearly faster to do it manually, but now either because our patterns have gotten more complicated, or browsers have improved their regexp engines, it's faster this way. It's also a lot shorter code-wise (though I wouldn't exactly call it nicer to look at, with that mile-long regexp!)
On Chrome, the old way (extended to ISO-8601) was taking ~2microsec per conversion, the new way is ~1.3microsec. FF is quite a bit slower at both versions, but the regexp is faster there too: ~6.8microsec vs ~10.7microsec.
The rest: use UTC milliseconds as the back end:
Changes our internal date linearization to use UTC rather than local milliseconds. Every day on a Plotly graph will now be 24 hours long, with no daylight shifts.
Note that for backward compatibility I still have us treat millisecond values (in ranges etc) as they were calculated previously, in local time, and shift them to UTC. I do the same thing with js date objects, on the theory that users generally construct these by using new Date(y, m, d, H, M, S, ms) with the date/time components they expect to see on the final graph.
If and when we add support for explicit timezone settings, we can do smarter things for users who specify a timezone, but I think this should remain the default behavior.
Fixes #171 and #811
This PR supersedes #1158 and #1172